Shared storage #1912
Shared storage #1912
Conversation
|
🔧 The result from spotlessApply was committed to the PR branch. |
| * @throws IOException if the delegate ToDiskExporter could not be created. | ||
| */ | ||
| @Deprecated | ||
| public static SpanToDiskExporter create(SpanExporter delegate, StorageConfiguration config) |
There was a problem hiding this comment.
We've previously added breaking changes without adding deprecations, because this tool isn't stable yet, if you think it's worth doing so, I'm fine with it. However, I think it should be fine to just change the signature here to replace StorageConfiguration by Storage for now.
There was a problem hiding this comment.
There was a problem hiding this comment.
Makes sense 👍 I think the getBody() one was causing issues before because it's defined upstream (maybe those are sorted now), but apart from that, I don't see why we should keep deprecated methods for now.
|
@LikeTheSalad please review |
This reverts commit 8e1bf43.
| throws IOException { | ||
| FromDiskExporterImpl<LogRecordData> delegate = | ||
| FromDiskExporterImpl.<LogRecordData>builder() | ||
| .setFolderName(SignalTypes.logs.name()) |
There was a problem hiding this comment.
It's been a while since I worked on this, so apologies because I didn't notice it earlier, though I just recalled that each signal's set of files needs to be in its own folder, separated from other signal files, and that's why this property was set, and also why we needed a storage object per signal. I know from a first glance it looks strange, as I also was confused for a moment, and I think it's because the current architecture is not straightforward. I'm planning to work on enhancing it soon to make it cleaner, but for now, I'm afraid that if we merge these changes, it can cause issues, especially around serialization.
In theory, we could share the same storage object across a single signal's ToDiskExporter and FromDiskExporter objects, however, those are instantiated independently by the consumer, so to make it work we would have to create some sort of factory that focuses on providing both ToDiskExporter and FromDiskExporter instances for a single signal where we would internally ensure that the same storage object is used. And then we would have to make these create methods package private to prevent users from calling them, so that they don't provide the wrong storage object. Though I'm not sure if that would add much value compared to the current way of using the lib.
There was a problem hiding this comment.
Good point - I changed the PR so that the storage builder takes the signal type as an argument.
This should make clear that the storage should not be used across signal types.
| exportedFnSeen = x; | ||
| return exportFnResultToReturn.get(); | ||
| }; | ||
| toDiskExporter = new ToDiskExporter<>(serializer, exportFn, storage, true); |
There was a problem hiding this comment.
It might be a small thing but I really like when a boolean parameter is removed, cheers!
3 participants
Possible fix for #1907